Stabilizing multivariate normal approximation #380
Conversation
|
Thanks! Is it intentional that this is to be merged into the |
|
Thanks for pointing that out, it was not intentional. It was actually developed against the current dev branch, so I changed it accordingly.
I will try to reproduce the error outside of BayesFlow. |
|
Pair programming with Valentin was successful (thx!) and we smoothed out the flaky tests. All backends support estimation of multivariate normal parameters now. I removed some rough edges, added tests, so hopefully the codecov/path tests pass too now. The PR is ready for review as soon as tests passed. EDIT: tests passed, ready for review. |
bayesflow/scores/scoring_rule.py
Outdated
| self.subnets_kwargs = subnets_kwargs or {} | ||
| self.links = links or {} | ||
|
|
||
| self.not_transforming_like_vector = [] |
There was a problem hiding this comment.
Please add a comment documenting what this should contain, so people who want to set this in a subclass know what to use
There was a problem hiding this comment.
+1, I would also like to know if we can handle this in a better way than a special attribute.
There was a problem hiding this comment.
Do we want this to be variable within a class, i.e., does this have to be an instance variable? If not, a constant class variable might be an option, like
class ScoringRule:
#: This variable lists ... (the #: syntax should allow sphinx to parse this)
NOT_TRANSFORMING_LIKE_VECTOR = tuple() # use immutable type tuple instead of listThere was a problem hiding this comment.
Good idea to use a class variable for this! Implemented it in d87b0b9.
I added documenting comments as well.
There was a problem hiding this comment.
Regarding a better way of handling this in general:
There is a discussion in #304 about the long term plans for special estimators and how they play with adapter transformations.
In my opinion, what we have here suffices for now and is a reasonable first step.
There was a problem hiding this comment.
Code overall looks good. See individual comments. I don't know enough about the requirements of this task to give a definite approval, but I trust that @han-ol did their research on this 🙂
Please do not merge before the individual comments are resolved, though.
| def _prepare_conditions(self, conditions: dict[str, np.ndarray], **kwargs) -> dict[str, Tensor]: | ||
| """Adapts and converts the conditions to tensors.""" | ||
| conditions = self.adapter(conditions, strict=False, stage="inference", **kwargs) | ||
| conditions.pop("inference_variables", None) |
There was a problem hiding this comment.
We could add this function to the ContinuousApproximator, if it is identical between it and the Point Approximator
There was a problem hiding this comment.
Yes! This and similar refactoring of the ContinuousApproximator is a good idea (but I would keep them out of this PR).
There is also the option of moving the conversion to tensor into the adapter. Possibly with an optional bool flag convert_to_tensor that is by default False.
bayesflow/scores/scoring_rule.py
Outdated
| self.subnets_kwargs = subnets_kwargs or {} | ||
| self.links = links or {} | ||
|
|
||
| self.not_transforming_like_vector = [] |
There was a problem hiding this comment.
+1, I would also like to know if we can handle this in a better way than a special attribute.
|
Thank you for your helpful reviews! I resolved the conversations concerning things that are unequivocally solved. What is left open is for you to check if you are happy with the state of things. From my side, the PR is ready to merge. |
5 participants
This PR seeks to adress the stability of estimating mean and covariance using
It is fixed by using link function called PositiveDefinite, that is based on the Cholesky decomposition.